W-2129587: removed wrapperStyle for LoadingSpinner to have full screen overlay#3696
W-2129587: removed wrapperStyle for LoadingSpinner to have full screen overlay#3696nayanavishwa wants to merge 1 commit intot/team404/sfp-on-pwafrom
Conversation
nayanavishwa
requested review from
alafemina and
amittapalli
and removed request for
a team
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
| return ( | ||
| <CheckoutProvider> | ||
| {isDeletingUnavailableItem && <LoadingSpinner wrapperStyles={{height: '100vh'}} />} | ||
| {isDeletingUnavailableItem && <LoadingSpinner />} |
There was a problem hiding this comment.
Not sure we should change this. I understand why you would identify the case though.
If this one existed before our branch, was it copied from yet another spinner case? I just wonder if there's a reason why it was done this way, which only fails for our payments case for some reason. After all 100vh is 100% of the viewport height.
There was a problem hiding this comment.
Screen.Recording.2026-02-27.at.11.37.02.AM.mov
Please check the recording. The issue might be more at the LoadingSpinner component level (LoadingSpinner -> Box component)where the position has to be fixed. I understand that changing at the component level will require testing through out the application.
The other alternative fix is:
<LoadingSpinner wrapperStyles={{ position: 'fixed' }} />
Let me know if you agree with this change. I will go ahead and update the PR.
The only other deviation I found in our code was checkout/index.jsx has used LoadingSpinner twice. I changed it to use it only once at CheckoutContainer, but this change did not solve the overlay issue.
There was a problem hiding this comment.
I don't really know the answer. I suggest invite the PWA Kit team (or whoever owns the spinner common component) to weigh in on the best practice. If there's an issue with the spinner maybe they can take it on and we fix/close our own bug when they've fixed it. If it's really just the wrapperStyles they should be able to choose the right value for it, and then we can simply apply that to our owned uses.
3 participants
Description
Bug:
Fix:
Types of Changes
Changes
How to Test-Drive This PR
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization